Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Vendor rcl_interfaces as a private module into rclrs #288

Merged
merged 3 commits into from
Oct 31, 2022
Merged

Vendor rcl_interfaces as a private module into rclrs #288

merged 3 commits into from
Oct 31, 2022

Conversation

nnmm
Copy link
Contributor

@nnmm nnmm commented Oct 27, 2022 •
edited
Loading

The parameter services require the rcl_interfaces message and service types. I think this option is preferable to publishing rcl_interfaces on crates.io.

The script included in this PR produces the vendor module inside rclrs by copying the generated code for the rcl_interfaces package and its dependency builtin_interfaces and adjusting the submodule paths in the code.
If these packages, or the rosidl_generator_rs package, get changed, we can update the vendor module by running this script.

@nnmm nnmm force-pushed the vendor_interfaces_for_parameter_service branch from fa688e0 to 4d09e19 Compare October 27, 2022 21:37
@esteve
Copy link
Collaborator

esteve commented Oct 28, 2022

LGTM, but I'll leave it to @jhdcs to approve it as I see that CI is failing.

@nnmm can you update the description of this PR and add a docstring to the Python script with a description of what it does and how to use it? Thanks

For a future PR, or maybe even this one, I'd add the test_interfaces messages so that we can integrate the tests here, I prefer tests to be in the same crate/package, they are easier to maintain and also serve as examples of how to use the API

@nnmm
Copy link
Contributor Author

nnmm commented Oct 28, 2022

@esteve I had planned to just create an alias ExampleMessage for one of the rcl_interfaces messages that we can use in doctests. For most tests, the contents of the message being used don't matter. So I propose to do that for now, and if/when we see a need for test_msgs specifically, we can consider vendoring that too.

@nnmm
Vendor rcl_interfaces as a private module into rclrs
53a4047 
The parameter services require the rcl_interfaces message and service types.
@nnmm nnmm force-pushed the vendor_interfaces_for_parameter_service branch from 4d09e19 to 53a4047 Compare October 28, 2022 11:39
jhdcs
jhdcs previously approved these changes Oct 28, 2022
View reviewed changes
Copy link
Collaborator

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me as well, though I don't see a failing CI. Was that a previous commit?

@nnmm
Copy link
Contributor Author

nnmm commented Oct 28, 2022

@jhdcs Yes, that was a previous commit. I've also added the comment requested by @esteve.

@nnmm nnmm requested a review from a team October 28, 2022 16:15
@nnmm
Add rcl_interfaces and builtin_interfaces to package.xml, since they …
4a35fb8 
…need to be present when rclrs is built and run
@nnmm
Copy link
Contributor Author

nnmm commented Oct 31, 2022

@jhdcs Could you reapprove this? I've added a commit to add these dependencies to package.xml, since we will still depend on the C libraries from these packages.

@nnmm nnmm merged commit 27342d5 into main Oct 31, 2022
@delete-merged-branch delete-merged-branch bot deleted the vendor_interfaces_for_parameter_service branch October 31, 2022 11:55
@nnmm
Copy link
Contributor Author

nnmm commented Oct 31, 2022

Thanks!

@maspe36 maspe36 mentioned this pull request Oct 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhdcs jhdcs jhdcs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nnmm @esteve @jhdcs